-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add finalize subroutine for stochastic physics #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I would strongly suggest adding
do:
This better protects the code from possible and difficult to chase segmentation faults in the future -- at least based on my experience. |
@HenryWinterbottom-NOAA good point, I thought about doing it, but was just lazy. |
@pjpegion If you are planning to do it, just send me a note (or submit a new PR) and I am happy to approve. |
@HenryWinterbottom-NOAA I added the if (allocated()) conditionals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @pjpegion.
Approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks okay; there are several places where a comment line
! Copy blocked data into contiguous arrays; no need to copy weights in (intent(out))
was moved around to places where it doesn't belong. I recommend removing or changing them, but certainly not a show stopper.
I am currently creating new baselines on cheyenne.intel, cheyenne.gnu and gaea.intel. The new baseline date tag will be 20201127, please update RTPWD
in rt.sh
accordingly.
Looks like the changes you are suggesting are for the fv3atm PR. |
Yes, sorry. |
This a new feature to add a finalize subroutine to deallocate array at the end of a run. Associated with PRs
NOAA-EMC/fv3atm#199 and
ufs-community/ufs-weather-model#278